Skip to content

Update docker.sh to add a check for existing image before building#17

Open
tianlelyd wants to merge 7 commits intoykdojo:masterfrom
tianlelyd:my-new-feature
Open

Update docker.sh to add a check for existing image before building#17
tianlelyd wants to merge 7 commits intoykdojo:masterfrom
tianlelyd:my-new-feature

Conversation

@tianlelyd
Copy link

  • Updated docker.sh to use the modification timestamp of Dockerfile to determine if the image needs to be rebuilt.
  • Added .last_build_timestamp to .gitignore with a comment explaining why it should be ignored.

ykdojo added 2 commits June 24, 2023 03:42
- Added a check to see if the Docker image already exists before building it. This is to optimize the script by avoiding unnecessary image builds, which can be time-consuming, and directly using the existing image if it's already available.
- Updated docker.sh to use the modification timestamp of Dockerfile to determine if the image needs to be rebuilt.
- Added .last_build_timestamp to .gitignore with a comment explaining why it should be ignored.
@ykdojo
Copy link
Owner

ykdojo commented Jun 24, 2023

Can you explain what the timestamp is for exactly?

@tianlelyd
Copy link
Author

use the modification timestamp of Dockerfile to determine if the image needs to be rebuilt.
If the Dockerfile file is modified, rebuild the image, and if not, use the image that has already been built

@ykdojo
Copy link
Owner

ykdojo commented Jun 25, 2023 via email

@ykdojo
Copy link
Owner

ykdojo commented Jun 25, 2023

I meant > or < instead of !=

@tianlelyd
Copy link
Author

I meant > or < instead of !=

My previous idea was to rebuild the image as soon as the Dockerfile file was modified, but in fact, whenever the file is modified, the modification time is usually greater than the previous timestamp! So using '>' may be more accurate
One more question, do you think this test is necessary? As I used it, I noticed that it would cost more time to build every time, so I submitted this change!

@ykdojo
Copy link
Owner

ykdojo commented Jun 25, 2023

That's interesting because it seems to only take <1s pretty much every time for me:
image

This is not the case for you?

@tweyew436
Copy link

Your project reminds me of an idea I had - maybe we could collaborate?

@ykdojo
Copy link
Owner

ykdojo commented Jun 25, 2023

@tweyew436 can you describe it and put it in an appropriate place?

@tianlelyd
Copy link
Author

That's interesting because it seems to only take <1s pretty much every time for me: image

This is not the case for you?

That's interesting because it seems to only take <1s pretty much every time for me: image

This is not the case for you?

7664825b-a5c7-43c6-be83-1e2a4f90d5ea
As shown in the figure, the time consumption of my side tested is about 4s

- Changed the timestamp comparison in docker.sh to use greater than operator for clarity and to avoid unnecessary builds.
@ykdojo
Copy link
Owner

ykdojo commented Jun 25, 2023

It looks good to me although I'd ideally like input from someone else. So I'll leave this open for now until we get someone else to take a look, hopefully

@tianlelyd
Copy link
Author

ok

ykdojo and others added 4 commits July 1, 2023 07:51
…conflicts with other services

🐛 fix(ai-plugin.json): update the API URL to use the new exposed port 9999 instead of 3000
🐛 fix(openapi.yaml): update the server URL to use the new exposed port 9999 instead of 3000
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments